RFC: Breaking API Change: drivers: can: rework support for manual bus-off recovery #69460
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
henrikbrixandersen
added
RFC
henrikbrixandersen
force-pushed
the
can_manual_vs_auto_recover_mode
branch
4 times, most recently
from
933a96b to
629a39b
Compare
henrikbrixandersen
force-pushed
the
can_manual_vs_auto_recover_mode
branch
from
629a39b to
453e4d9
Compare
henrikbrixandersen
added
the
Release Notes Required
henrikbrixandersen
changed the title
henrikbrixandersen
added
the
Breaking API Change
henrikbrixandersen
force-pushed
the
can_manual_vs_auto_recover_mode
branch
3 times, most recently
from
36cc408 to
d4e44a6
Compare
henrikbrixandersen
removed
the
Release Notes Required
henrikbrixandersen
requested review from
aaillet,
karstenkoenig,
manuargue,
erwango,
ABOSTM,
FRASTM,
mmahadevan108 and
dleach02
as code owners
zephyrbot
requested review from
alexanderwachter,
danieldegrasse and
Desvauxm-st
zephyrbot
requested review from
gautierg-st,
GeorgeCGV,
martinjaeger and
str4t0m
henrikbrixandersen
force-pushed
the
can_manual_vs_auto_recover_mode
branch
2 times, most recently
from
e645eb0 to
8992d05
Compare
|
RFC ready for review. |
martinjaeger
previously approved these changes
Feb 29, 2024
Comment on lines
+357
to
+369
| can_mode_t supported = CAN_MODE_LOOPBACK | CAN_MODE_LISTENONLY; | ||
| struct can_mcan_data *data = dev->data; | ||
| uint32_t cccr; | ||
| uint32_t test; | ||
| int err; | ||
|
|
||
| #ifdef CONFIG_CAN_FD_MODE | ||
| if ((mode & ~(CAN_MODE_LOOPBACK | CAN_MODE_LISTENONLY | CAN_MODE_FD)) != 0U) { | ||
| LOG_ERR("unsupported mode: 0x%08x", mode); | ||
| return -ENOTSUP; | ||
| if (IS_ENABLED(CONFIG_CAN_MANUAL_RECOVERY_MODE)) { | ||
| supported |= CAN_MODE_MANUAL_RECOVERY; | ||
| } | ||
| #else /* CONFIG_CAN_FD_MODE */ | ||
| if ((mode & ~(CAN_MODE_LOOPBACK | CAN_MODE_LISTENONLY)) != 0U) { | ||
|
|
||
| if (IS_ENABLED(CONFIG_CAN_FD_MODE)) { | ||
| supported |= CAN_MODE_FD; | ||
| } |
There was a problem hiding this comment.
Why not call can_mcan_get_capabilities here in order to avoid code duplication?
There was a problem hiding this comment.
I agree, it seems that all drivers can benefit from this and avoid issues like #69460 (comment) :)
There was a problem hiding this comment.
Thanks, yes - I too noticed that pattern when converting the drivers. I didn't want to mix that change into this PR, but I'm preparing to move this generic check into the can_set_mode() implementation function itself.
dleach02
previously approved these changes
Feb 29, 2024
manuargue
approved these changes
Mar 1, 2024
|
Look good to me. zephyr/drivers/can/can_nxp_s32_canxl.c Line 153 in 8992d05 |
manuargue
requested changes
Mar 1, 2024
henrikbrixandersen
dismissed stale reviews from dleach02 and martinjaeger
via
f5b8907
Nicely spotted, thanks. I have updated the patch. |
henrikbrixandersen
force-pushed
the
can_manual_vs_auto_recover_mode
branch
from
f5b8907 to
71f1775
Compare
Since all CAN controllers drivers seem to support automatic recovery (for any future drivers for hardware without this hardware capability this can easily be implemented in the driver), change the Zephyr CAN controller API policy to: - Always enable automatic bus recovery upon driver initialization, regardless of Kconfig options. Since CAN controllers are initialized in "stopped" state, no unwanted bus-off recovery will be started at this point. - Invert and rename the Kconfig CONFIG_CAN_AUTO_BUS_OFF_RECOVERY, which is enabled by default, to CONFIG_CAN_MANUAL_RECOVERY_MODE, which is disabled by default. Enabling CONFIG_CAN_MANUAL_RECOVERY_MODE=y enables support for the can_recover() API function and a new manual recovery mode (see next bullet). Keeping this guarded by Kconfig allows keeping the flash footprint down for applications not using manual bus-off recovery. - Introduce a new CAN controller operational mode CAN_MODE_MANUAL_RECOVERY. Support for this is only enabled if CONFIG_CAN_MANUAL_RECOVERY_MODE=y. Having this as a mode allows applications to inquire whether the CAN controller supports manual recovery mode via the can_get_capabilities() API function and either fail or rely on automatic recovery - and it allows CAN controller drivers not supporting manual recovery mode to fail early in can_set_mode() during application startup instead of failing when can_recover() is called at a later point in time. Signed-off-by: Henrik Brix Andersen <[email protected]>
List the changes to the CAN bus-off recovery functionality. Signed-off-by: Henrik Brix Andersen <[email protected]>
henrikbrixandersen
force-pushed
the
can_manual_vs_auto_recover_mode
branch
from
71f1775 to
9426740
Compare
|
Rebased on |
martinjaeger
approved these changes
Mar 1, 2024
manuargue
approved these changes
Mar 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since all CAN controllers drivers seem to support automatic recovery (for any future drivers for hardware without this hardware capability this can easily be implemented in the driver), change the Zephyr CAN controller API policy to:
Always enable automatic bus recovery upon driver initialization, regardless of Kconfig options. Since CAN controllers are initialized in "stopped" state, no unwanted bus-off recovery will be started at this point.
Invert and rename the Kconfig CONFIG_CAN_AUTO_BUS_OFF_RECOVERY, which is enabled by default, to CONFIG_CAN_MANUAL_RECOVERY_MODE, which is disabled by default. Enabling CONFIG_CAN_MANUAL_RECOVERY_MODE=y enables support for the can_recover() API function and a new manual recovery mode (see next bullet). Keeping this guarded by Kconfig allows keeping the flash footprint down for applications not using manual bus-off recovery.
Introduce a new CAN controller operational mode CAN_MODE_MANUAL_RECOVERY. Support for this is only enabled if CONFIG_CAN_MANUAL_RECOVERY_MODE=y. Having this as a mode allows applications to inquire whether the CAN controller supports manual recovery mode via the can_get_capabilities() API function and either fail or rely on automatic recovery - and it allows CAN controller drivers not supporting manual recovery mode to fail early in can_set_mode() during application startup instead of failing when can_recover() is called at a later point in time.
RFC: #69459